Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat]: Timesheet-Duration-Display #3352

Merged
merged 5 commits into from
Nov 21, 2024
Merged

Conversation

Innocent-Akim
Copy link
Contributor

@Innocent-Akim Innocent-Akim commented Nov 20, 2024

Description

Please include a summary of the changes and the related issue.
#3043

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Previous screenshots

Please add here videos or images of previous status

Current screenshots

Please add here videos or images of previous status

Summary by CodeRabbit

  • New Features

    • Introduced DisplayTimeForTimesheet, TotalTimeDisplay, and TotalDurationByDate components for enhanced time display in timesheets.
    • Updated DataTableTimeSheet component to dynamically show total hours and task durations.
  • Bug Fixes

    • Minor syntactical adjustment in useTimelogFilterOptions to improve code readability without affecting functionality.
  • Documentation

    • Improved clarity and maintainability of the code with better-structured components for time display.

Copy link
Contributor

coderabbitai bot commented Nov 20, 2024

Walkthrough

The changes include a minor syntactical adjustment in the useTimelogFilterOptions function and significant enhancements to the DataTableTimeSheet component and the task-displays module. The DataTableTimeSheet component now utilizes new display components for time management, improving readability and encapsulation of time-related logic. Additionally, three new functions for displaying time based on task durations and timesheet logs have been introduced in the task-displays module, enhancing its functionality.

Changes

File Path Change Summary
apps/web/app/hooks/features/useTimelogFilterOptions.ts Added a comma after the setSelectTimesheet property in the returned object.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx Added imports for DisplayTimeForTimesheet, TotalTimeDisplay, and TotalDurationByDate. Updated rendering logic for total duration and task details, modified Badge content, and refined structure for readability.
apps/web/lib/features/task/task-displays.tsx Added imports for TimesheetLog, formatDate, secondsToTime, and ClockIcon. Introduced DisplayTimeForTimesheet, TotalTimeDisplay, and TotalDurationByDate functions for enhanced time display capabilities.

Possibly related PRs

Suggested reviewers

  • evereq

🐇 In the code, a change was made,
A little comma, not a big cascade.
New displays for time, oh what a sight,
Making our timesheets feel just right.
With functions to show hours, neat and clear,
Hopping along, let’s give a cheer! 🎉

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/web/app/hooks/features/useTimelogFilterOptions.ts

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
apps/web/lib/features/task/task-displays.tsx (1)

67-98: Consider creating a dedicated time-display module

The new time display components (DisplayTimeForTimesheet and TotalTimeDisplay) seem to be more focused on time visualization rather than task-specific display logic. Consider moving them to a dedicated time-display module (e.g., time-displays.tsx) to:

  1. Better separate concerns
  2. Make these components reusable across different contexts
  3. Keep task-displays.tsx focused on task-specific display logic
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)

334-336: Verify duration prop type and add error handling

While the dedicated component improves code organization, consider:

  1. Adding prop type validation for duration
  2. Handling cases where duration is undefined or invalid
 <DisplayTimeForTimesheet
-  duration={task.timesheet.duration}
+  duration={task.timesheet.duration ?? 0}
 />
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bc63849 and 7fc7b0e.

📒 Files selected for processing (3)
  • apps/web/app/hooks/features/useTimelogFilterOptions.ts (1 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4 hunks)
  • apps/web/lib/features/task/task-displays.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/web/app/hooks/features/useTimelogFilterOptions.ts
🔇 Additional comments (4)
apps/web/lib/features/task/task-displays.tsx (1)

1-1: LGTM! Import statements are well-structured

The new imports are appropriately organized and use well-maintained dependencies.

Also applies to: 5-6

apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)

271-276: LGTM! Improved code clarity and type safety

The changes enhance readability by using explicit return statements and proper type handling for the status.


58-58: Verify component imports from task-displays module

The new time display components align with the PR objectives. Let's verify their implementation.


294-295: Verify translation key and time calculation logic

The changes improve i18n support and encapsulate time calculation logic. However, let's verify:

  1. The translation key 'timer.TOTAL_HOURS' exists
  2. The TotalTimeDisplay component handles edge cases (empty array, null values)
✅ Verification successful

Let me gather more information about the TotalTimeDisplay component implementation since the previous search didn't show its details.


Let me try a different search pattern to find the TotalTimeDisplay component.


Translation key and component implementation verified successfully

The verification shows:

  1. The translation key 'timer.TOTAL_HOURS' exists in all language files under both apps/web/locales/ and apps/desktop/i18n/
  2. The TotalTimeDisplay component properly handles edge cases:
    • Checks if timesheetLog is an array using Array.isArray()
    • Provides fallback to 0 for non-array input
    • Handles null/undefined duration with totalDuration || 0
    • Properly formats hours and minutes with padStart
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify translation key and component implementation
# Check for translation key
rg -g '*.json' '"TOTAL_HOURS"'

# Check TotalTimeDisplay implementation for edge cases
ast-grep --pattern 'export function TotalTimeDisplay({ $_ }) {
  $$$
}'

Length of output: 2583


Script:

#!/bin/bash
# Search for TotalTimeDisplay component implementation
rg -g '*.{tsx,ts,jsx,js}' -B 2 -A 10 'TotalTimeDisplay.*=.*{' 

Length of output: 171


Script:

#!/bin/bash
# Search for TotalTimeDisplay component implementation
rg -g '*.{tsx,ts,jsx,js}' -B 2 -A 10 'function TotalTimeDisplay'

# Alternative search in case it's defined as an arrow function
rg -g '*.{tsx,ts,jsx,js}' -B 2 -A 10 'const TotalTimeDisplay'

Length of output: 1219

apps/web/lib/features/task/task-displays.tsx Outdated Show resolved Hide resolved
apps/web/lib/features/task/task-displays.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
apps/web/lib/features/task/task-displays.tsx (3)

1-6: Maintain consistent quote style in imports

The ClockIcon import uses double quotes while other imports use single quotes.

-import { ClockIcon } from "@radix-ui/react-icons"
+import { ClockIcon } from '@radix-ui/react-icons';

68-74: Add TypeScript return type for better type safety

The formatTime helper function would benefit from explicit return type annotation.

-const formatTime = (hours: number, minutes: number) => (
+const formatTime = (hours: number, minutes: number): JSX.Element => (

83-85: Maintain consistent quote style in className attributes

Use double quotes for consistency with other components in the file.

-<div className='flex items-center font-medium gap-x-1'>
-	<ClockIcon className='text-green-400 text-[14px] h-4 w-4' />
-	<div className='flex items-center'>
+<div className="flex items-center font-medium gap-x-1">
+	<ClockIcon className="text-green-400 text-[14px] h-4 w-4" />
+	<div className="flex items-center">
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7fc7b0e and 9b8ff14.

📒 Files selected for processing (1)
  • apps/web/lib/features/task/task-displays.tsx (2 hunks)
🔇 Additional comments (2)
apps/web/lib/features/task/task-displays.tsx (2)

93-103: Implementation looks good!

The component is well-implemented with:

  • Proper memoization using React.memo
  • Null safety in duration calculation
  • Reuse of the formatTime helper
  • Component displayName for debugging

76-103: Verify the component usage in DataTableTimeSheet

Let's verify that these components are properly integrated in the DataTableTimeSheet component as mentioned in the AI summary.

✅ Verification successful

Components are properly integrated and used in DataTableTimeSheet

The verification confirms that both components are correctly imported and used in the table-time-sheet.tsx file:

  • DisplayTimeForTimesheet is used to show individual timesheet durations
  • TotalTimeDisplay is used to display the total duration of timesheet logs
  • Both components are working with the expected props and data structure
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of DisplayTimeForTimesheet and TotalTimeDisplay components

# Search for component imports and usage
echo "Checking component imports and usage..."
rg -l "DisplayTimeForTimesheet|TotalTimeDisplay" --type tsx

# Search specifically in DataTableTimeSheet component
echo "Checking DataTableTimeSheet implementation..."
rg -A 5 "DataTableTimeSheet" --type tsx

Length of output: 347


Script:

#!/bin/bash
# Description: Verify the usage of DisplayTimeForTimesheet and TotalTimeDisplay components

# Search for component imports and usage without file type restriction
echo "Checking component imports and usage..."
rg -l "DisplayTimeForTimesheet|TotalTimeDisplay"

# Search specifically in DataTableTimeSheet component
echo "Checking DataTableTimeSheet implementation..."
rg -A 10 "DataTableTimeSheet"

# Search for files that might contain these components
echo "Searching for potential component files..."
fd -e tsx -e ts -e jsx -e js

Length of output: 74670


Script:

#!/bin/bash
# Search for the usage of DisplayTimeForTimesheet and TotalTimeDisplay components in table-time-sheet.tsx
echo "Checking component usage in table-time-sheet.tsx..."
rg -A 5 "DisplayTimeForTimesheet|TotalTimeDisplay" "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"

# Search for any other potential usages of these components
echo "Checking other potential usages..."
rg -A 5 "DisplayTimeForTimesheet|TotalTimeDisplay" "apps/web/lib/features/task/task-displays.tsx"

Length of output: 2238

@Innocent-Akim Innocent-Akim requested a review from evereq November 20, 2024 07:16
@Innocent-Akim Innocent-Akim self-assigned this Nov 20, 2024
@Innocent-Akim Innocent-Akim removed the request for review from evereq November 20, 2024 07:56
@Innocent-Akim Innocent-Akim added the Don't Merge Do not merge this PR until label not removed label Nov 20, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
apps/web/lib/features/task/task-displays.tsx (4)

1-6: Maintain consistent quote style in imports

The ClockIcon import uses double quotes while other imports use single quotes.

-import { ClockIcon } from "@radix-ui/react-icons"
+import { ClockIcon } from '@radix-ui/react-icons';

83-85: Maintain consistent quote style in className attributes

Use double quotes for consistency with other components in the file.

-		<div className='flex items-center font-medium gap-x-1'>
-			<ClockIcon className='text-green-400 text-[14px] h-4 w-4' />
-			<div className='flex items-center'>
+		<div className="flex items-center font-medium gap-x-1">
+			<ClockIcon className="text-green-400 text-[14px] h-4 w-4" />
+			<div className="flex items-center">

106-120: Consider improving error handling and date comparisons

A few suggestions to enhance robustness and performance:

  1. Add error handling for invalid dates
  2. Consider timezone implications in date comparisons
  3. Move calculations outside JSX for better performance
 export const TotalDurationByDate = React.memo(
 	({ timesheetLog, createdAt }: { timesheetLog: TimesheetLog[]; createdAt: Date | string }) => {
-		const targetDateISO = new Date(createdAt).toISOString();
+		// Handle invalid dates
+		const targetDate = new Date(createdAt);
+		if (isNaN(targetDate.getTime())) {
+			console.warn('Invalid date provided to TotalDurationByDate');
+			return null;
+		}
+
+		// Move calculations outside JSX
+		const targetDateISO = targetDate.toISOString();
+		
+		// Use UTC date comparison to avoid timezone issues
 		const filteredLogs = timesheetLog.filter(
-			(item) => formatDate(item.timesheet.createdAt) === formatDate(targetDateISO));
+			(item) => {
+				const itemDate = new Date(item.timesheet.createdAt);
+				return itemDate.toDateString() === targetDate.toDateString();
+			});
+
 		const totalDurationInSeconds = filteredLogs.reduce(
 			(total, log) => total + (log.timesheet?.duration || 0), 0);
 		const { h: hours, m: minutes } = secondsToTime(totalDurationInSeconds);
+
 		return (
 			<div className="flex items-center text-[#868688]">
 				{formatTime(hours, minutes)}

Line range hint 1-121: Consider extracting shared types and constants

To improve maintainability and reusability, consider:

  1. Creating a shared types file for timesheet-related interfaces
  2. Extracting time-related constants and utilities into a dedicated module
  3. Creating a shared styles object for common className combinations

This would help maintain consistency across the codebase and make future updates easier.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9b8ff14 and 025c28b.

📒 Files selected for processing (2)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4 hunks)
  • apps/web/lib/features/task/task-displays.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (1)
apps/web/lib/features/task/task-displays.tsx (1)

68-74: LGTM! Well-structured utility function

Good job extracting the common time formatting logic into a reusable function with proper typing.

@Innocent-Akim Innocent-Akim removed the Don't Merge Do not merge this PR until label not removed label Nov 20, 2024
@Innocent-Akim Innocent-Akim requested a review from evereq November 20, 2024 08:06
@evereq
Copy link
Member

evereq commented Nov 20, 2024

@Innocent-Akim please work with @NdekoCode to carefully fix merge conflict as there is a lot of changes in one file you both did :(

@evereq evereq marked this pull request as draft November 20, 2024 21:44
@Innocent-Akim Innocent-Akim force-pushed the feat/timesheet-duration-display branch from 025c28b to fc23dca Compare November 21, 2024 10:23
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
apps/web/lib/features/task/task-displays.tsx (4)

6-6: Maintain consistent quote style

The ClockIcon import uses double quotes while other imports use single quotes. Maintain consistency by using single quotes.

-import { ClockIcon } from "@radix-ui/react-icons"
+import { ClockIcon } from '@radix-ui/react-icons'

76-91: Clean up className styling

The component implementation looks good, but there are some style inconsistencies:

  1. Inconsistent quote style in className attributes
  2. Redundant text-[14px] alongside h-4 w-4
-		<div className='flex items-center font-medium gap-x-1'>
-			<ClockIcon className='text-green-400 text-[14px] h-4 w-4' />
-			<div className='flex items-center'>
+		<div className="flex items-center font-medium gap-x-1">
+			<ClockIcon className="text-green-400 h-4 w-4" />
+			<div className="flex items-center">

106-121: Consider optimizing date handling and performance

While the implementation is functional, consider these improvements:

  1. Date comparison might be affected by timezone differences. Consider using start/end of day comparisons.
  2. For large datasets, consider memoizing the filtered results.
export const TotalDurationByDate = React.memo(
	({ timesheetLog, createdAt }: { timesheetLog: TimesheetLog[]; createdAt: Date | string }) => {
-		const targetDateISO = new Date(createdAt).toISOString();
+		const targetDate = new Date(createdAt);
+		const startOfDay = new Date(targetDate.setHours(0, 0, 0, 0));
+		const endOfDay = new Date(targetDate.setHours(23, 59, 59, 999));
+
+		// Memoize filtered logs
+		const filteredLogs = React.useMemo(() => 
			timesheetLog.filter((item) => {
-				formatDate(item.timesheet.createdAt) === formatDate(targetDateISO)
+				const logDate = new Date(item.timesheet.createdAt);
+				return logDate >= startOfDay && logDate <= endOfDay;
			}),
+			[timesheetLog, startOfDay, endOfDay]
+		);

Line range hint 1-121: Good architectural decisions and component structure

The file demonstrates good architectural decisions:

  1. Clear separation of concerns between display components
  2. Proper use of memoization for performance
  3. Good extraction of common functionality
  4. Consistent error handling

Consider creating a separate utilities file for time-related functions if more time manipulation logic is added in the future.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 025c28b and 375f14e.

📒 Files selected for processing (3)
  • apps/web/app/hooks/features/useTimelogFilterOptions.ts (1 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4 hunks)
  • apps/web/lib/features/task/task-displays.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/web/app/hooks/features/useTimelogFilterOptions.ts
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (2)
apps/web/lib/features/task/task-displays.tsx (2)

68-74: LGTM! Well-structured utility function

Good job extracting the common time formatting logic into a reusable function. This reduces code duplication and maintains consistency across components.


93-103: LGTM! Well-implemented memoized component

The component is well-structured with:

  • Proper memoization for performance
  • Null-safe duration calculation
  • Type-safe props
  • Reuse of formatTime utility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants